Skip to content

Protect host system trees from runtime binds - #13

Open
omry wants to merge 1 commit into
pr12from
pr13
Open

Protect host system trees from runtime binds#13
omry wants to merge 1 commit into
pr12from
pr13

Conversation

@omry

@omry omry commented Aug 3, 2026

Copy link
Copy Markdown
Owner

Reject runtime host sources that resolve to the filesystem root or protected kernel system trees. Use platform-specific filesystem identity to catch aliases while preserving explicit ordinary directory binds with nested special objects.

Document the narrowed direct-bind security contract, close the corresponding backlog item, and add a security changelog fragment.


Stack created with Sapling. Best reviewed with ReviewStack.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 75d08ecf23

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

}

func protectedRuntimeHostTreeV1(hostPath string) (string, error) {
resolved, err := filepath.EvalSymlinks(hostPath)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Make protected-source validation race-free

When another process can rename or retarget a configured source symlink, it can change the link from an ordinary path to /, /proc, /dev, or /sys after EvalSymlinks returns but before Docker resolves the original source.HostPath. The validation therefore approves one object while command_execution.go later passes a different, mutable pathname to Docker, bypassing the protection this change adds; preserve and launch from a stable resolved object or otherwise prevent source replacement through container creation.

Useful? React with 👍 / 👎.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR hardens runtime host bind validation by rejecting host sources that resolve to the host filesystem root or protected kernel system trees/filesystems, including symlink and filesystem-identity aliases, and updates the design docs/backlog plus adds a security changelog fragment.

Changes:

  • Add host bind preflight rejection for filesystem-root binds and protected system trees (/proc, /dev, /sys) including symlink aliases.
  • Add platform-specific filesystem identity detection (Linux/macOS) to reject protected kernel filesystems even when mounted elsewhere.
  • Update controlled-session design documentation/backlog and add a Security changelog fragment.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
internal/dockerdeploy/runtime_host_preflight.go Adds protected-host-tree/filesystem detection to runtime host source validation.
internal/dockerdeploy/runtime_host_preflight_test.go Adds cross-platform tests for rejecting protected host trees and aliases.
internal/dockerdeploy/runtime_host_preflight_unix_test.go Adds Unix-only test ensuring explicit directory binds with nested special objects remain allowed.
internal/dockerdeploy/runtime_host_filesystem_linux.go Implements Linux filesystem identity checks (statfs + mountinfo/statx) for protected kernel filesystems.
internal/dockerdeploy/runtime_host_filesystem_linux_test.go Adds Linux tests for kernel filesystem detection and tmpfs/devtmpfs distinction behavior.
internal/dockerdeploy/runtime_host_filesystem_darwin.go Implements macOS filesystem identity checks for devfs/procfs.
internal/dockerdeploy/runtime_host_filesystem_other.go Provides no-op implementation for non-Linux/non-macOS platforms.
docs/CONTROLLED_SESSION_DESIGN.md Documents narrowed direct-bind security contract and updated root-runtime messaging.
docs/BACKLOG.md Updates backlog items reflecting completed/shifted security work and future network gateway work.
.changes/unreleased/+protect-host-system-trees.yaml Adds a Security changelog fragment describing the new bind rejections.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +133 to +136
return fmt.Errorf(
"runtime plan %q mount %q host source resolves to protected host system source %q; ordinary host binds cannot expose the host filesystem root, /proc, /dev, or /sys",
planID, mount.Destination, protected,
)
@omry

omry commented Aug 6, 2026

Copy link
Copy Markdown
Owner Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2083678b49

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +113 to +115
switch strings.ToLower(scheme) {
case "unix", "npipe":
return true

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Verify the daemon namespace, not just the socket scheme

When Reploy runs in a container with the host Docker socket mounted, or connects through a Unix-socket proxy, this branch accepts the endpoint even though the daemon can use a different mount namespace or machine. Docker resolves bind sources in the daemon namespace while ValidateRuntimeHostSourcesV1 inspected Reploy's namespace, so a path that is benign locally can resolve to /, /proc, /dev, or /sys for the daemon and bypass the new guard. Fresh evidence beyond the earlier remote-endpoint finding is that the newly added check classifies every Unix socket as local; require proof that the daemon shares the validated namespace or reject configurations where that cannot be established.

Useful? React with 👍 / 👎.

@omry

omry commented Aug 6, 2026

Copy link
Copy Markdown
Owner Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2e10d22577

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +305 to +307
if length == bestLength && identity != best {
return runtimeHostMountIdentityV1{}, fmt.Errorf("host path %q has ambiguous mountinfo identity", path)
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Resolve the visible mount when mount points are stacked

On Linux kernels that do not expose STATX_MNT_ID, a valid overmount leaves multiple mountinfo records with the same mount point, and this fallback treats those records as ambiguous instead of selecting the currently visible topmost mount. Consequently, any ordinary host bind at or below that mount point fails validation on older kernels. Unlike the earlier missing-mount-ID issue, the new path-based fallback now reaches this error; resolve the visible mount through the mount topology rather than rejecting equal-length matches.

Useful? React with 👍 / 👎.

@omry

omry commented Aug 6, 2026

Copy link
Copy Markdown
Owner Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 617a789293

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +26 to +27
if defaultDockerEndpointCheckV1.checked {
return nil

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Revalidate the endpoint before later Docker commands

When the active Docker context changes after the first successful executeDockerOutput call—for example, another command runs docker context use remote while the Reploy process remains active—this process-wide checked flag skips all subsequent endpoint inspection even though each Docker CLI invocation rereads the active context. Later image probes, builds, or cleanup commands can therefore run against the newly selected remote daemon despite the new remote-endpoint prohibition; either pin the verified endpoint for every command or revalidate it before execution.

Useful? React with 👍 / 👎.

Comment on lines +444 to +448
if !pathWithinV1(identity.mountPoint, protectedTree) || identity.device != candidate.device {
continue
}
if pathWithinV1(effective, identity.root) || pathWithinV1(identity.root, effective) {
return true, nil

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Ignore hidden mounts when matching protected trees

When a bind below /proc, /dev, or /sys is subsequently hidden by an overmount at the same location, /proc/self/mountinfo retains both records, but this loop treats the hidden record as still exposing its backing path through the protected tree. For example, if /secret was bound at /dev/x and /dev/x is then overmounted with another filesystem, an ordinary source of /secret is still rejected even though the only visible /dev/x mount is unrelated; filter these records through the visible-mount topology before testing backing-path overlap.

Useful? React with 👍 / 👎.

@omry

omry commented Aug 6, 2026

Copy link
Copy Markdown
Owner Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7b94f4d1ab

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +123 to +124
if err := requireLocalDockerEndpointV1(ctx, spec, timeout); err != nil {
return err

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Validate endpoints for bypassed Docker runners

When the installed system service starts, its actual compose up is passed to runCommandWithoutDockerPreflight from installed_service_container.go:101-108, so this endpoint check is never applied to the container-creation command. If the active context changes after the earlier image-reference probe, that command can target a remote daemon after host sources were validated locally, recreating the namespace mismatch this restriction is intended to prevent. Fresh evidence beyond the earlier cached-check finding is that removing the cache still leaves this independent command path bypassing endpoint validation entirely.

Useful? React with 👍 / 👎.

@omry

omry commented Aug 6, 2026

Copy link
Copy Markdown
Owner Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: bff9ea2245

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

environment,
options,
runCommandWithoutDockerPreflight,
runCommand,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Preflight absolute Docker paths before service launch

When the installed system service starts with a remote active Docker context, this replacement still bypasses endpoint validation: RunInstalledServiceContainerV1 requires dockerPath to be absolute, while runCommand invokes dockerPreflight only when spec.Name == "docker". The compose up therefore runs against the remote daemon after host sources were validated locally. Fresh evidence beyond the earlier bypass comment is that switching the runner to runCommand does not activate its preflight for the absolute executable path used here; either recognize the configured Docker executable or call endpoint validation explicitly.

Useful? React with 👍 / 👎.

@omry

omry commented Aug 6, 2026

Copy link
Copy Markdown
Owner Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 9249110975

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

if kind := protectedRuntimeHostFilesystemKindV1(filesystemMagic); kind != "" {
return kind, nil
}
protectedMount, err := runtimeHostSharesProtectedMountV1(path)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Reject bind sources containing protected submounts

When an otherwise ordinary source such as /safe contains a visible bind mount like /safe/root -> / or /safe/proc -> /proc, this check examines only /safe itself and runtimeHostSharesProtectedMountV1 only correlates mounts located beneath /proc, /dev, or /sys. The runtime renderers do not disable recursive bind behavior (execution_render.go:130-142 and command_execution.go:240-252), and Docker documents that bind mounts are recursive by default, so the protected nested mount is included in the container. Fresh evidence beyond the earlier exact-alias findings is that selecting the alias's parent still exposes the entire protected tree; inspect visible submounts below the selected source and reject any whose effective backing path exposes root or a protected tree.

Useful? React with 👍 / 👎.

@omry

omry commented Aug 6, 2026

Copy link
Copy Markdown
Owner Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

// runCommandWithoutDockerPreflight is for follow-up commands in one
// higher-level Docker operation whose first command already passed preflight.
// Callers must not use it as the entry point to an independent operation.

P1 Badge Route provider-install starts through endpoint preflight

For Docker-managed and Docker Desktop installs, provider_install_host_commands.go:38-49 builds an independent compose up command using the absolute Docker path, but provider_install_host_execute.go:41-54 still invokes it through this bypass runner. If the active context is remote, or changes after an earlier build probe, installation therefore creates the workload on that daemon while its host paths were validated locally, contrary to the new remote-endpoint restriction in docs/CONTROLLED_SESSION_DESIGN.md:691-698. Fresh evidence beyond the earlier installed-service comments is that this separate provider-install startup path remains unchecked; pass it through runDockerCommand.

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

return false, err
}
current := string(filepath.Separator)
for _, component := range strings.Split(strings.TrimPrefix(filepath.Clean(absolute), current), current) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Inspect symlinks before collapsing parent components

On Linux kernels without openat2, a source such as /safe/jump/.. where jump -> /proc/self/cwd is cleaned to /safe here before Lstat, so the fallback never notices the symlink. EvalSymlinks can then resolve the magic link relative to Reploy's benign working directory, while Docker receives the original path and can resolve it relative to the daemon (commonly exposing /), bypassing the protected-tree check and the documented fail-closed behavior for symlinked sources on older kernels. Walk the original components without lexically eliminating .., or reject such components in this fallback.

Useful? React with 👍 / 👎.

@omry

omry commented Aug 6, 2026

Copy link
Copy Markdown
Owner Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4fc7f6076f

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +189 to +190
if dev.Dev == root.Dev {
return false, nil

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Reject aliases of static rootfs-backed /dev

When /dev is a static directory on the root filesystem and is bind-mounted at an alias such as /safe/dev, this early return disables the device-filesystem comparison. EvalSymlinks cannot collapse the bind alias, Statfs reports the ordinary root filesystem, and mountinfo contains no canonical /dev mount identity to match, so validation accepts the alias and Docker exposes the host device nodes. Fresh evidence beyond the earlier tmpfs-backed /dev report is that the new same-device branch explicitly opts out on rootfs-backed /dev; compare the candidate's effective backing path with /dev even in this configuration.

Useful? React with 👍 / 👎.

Reject runtime host sources that resolve to the filesystem root or protected kernel system trees. Use platform-specific filesystem and mount topology identity to catch direct, aliased, and nested kernel-control filesystems while preserving explicit ordinary directory binds with nested application objects.

Reject unsupported remote Docker endpoints before runtime and recovered-container operations, including admission cleanup paths.

Document the narrowed direct-bind security contract, close the corresponding backlog item, and add a security changelog fragment.
@omry

omry commented Aug 6, 2026

Copy link
Copy Markdown
Owner Author

@codex review

Please review the exact current head 3cb1e885c476b819a150ebb7bbe32fdad78d36ec, including the amended remote-Docker preflight and protected kernel-filesystem enforcement.

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Keep them coming!

Reviewed commit: 3cb1e885c4

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved PR reviewed and approved

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants